Skip to content

Add SyncStatus field to RelatedResourceSpec#175

Open
iakmc wants to merge 3 commits into
kcp-dev:mainfrom
iakmc:sync-status-for-related-resources
Open

Add SyncStatus field to RelatedResourceSpec#175
iakmc wants to merge 3 commits into
kcp-dev:mainfrom
iakmc:sync-status-for-related-resources

Conversation

@iakmc

@iakmc iakmc commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Adds a syncStatus field to RelatedResourceSpec. When set to true, the status subresource of a related object is synced in the same direction as its spec:

  • origin: kcp -> status flows kcp -> service cluster
  • origin: service -> status flows service cluster -> kcp

This is independent of the existing syncStatusBack behavior on primary resources (which always syncs status in the reverse direction).

Example

related:
  - identifier: subnet
    origin: kcp
    group: kcp.example.com
    version: v1
    resource: subnets
    syncStatus: true   # kcp status -> service cluster

Changes

  • sdk/apis/syncagent/v1alpha1/published_resource.go - new SyncStatus bool on RelatedResourceSpec; adds a CEL validation rule that rejects origin: service + syncStatus: true without a watch configured
  • internal/sync/object_syncer.go - new syncStatusForward field and syncObjectStatusForward() method (returns error only); called unconditionally from syncObjectContents() so a simultaneous spec+status change doesn't defer the status sync by one reconciliation cycle; includes a 404 guard that emits a Warning event instead of error-looping when the destination CRD has no status subresource
  • internal/sync/object_syncer_test.go - unit tests for syncObjectStatusForward() (5 sub-tests covering no-op conditions, happy path, and 404 guard) and syncObjectContents() status-runs-even-on-spec-requeue invariant
  • test/e2e/sdk/cel_validations_test.go - three new cases in TestValidateRelatedResourceSpec covering rejection of origin: service + syncStatus: true without watch, and the two valid combinations
  • internal/sync/syncer_related.go - wires SyncStatus into objectSyncer: sets syncStatusForward and adds "status" to subresources
  • deploy/crd/kcp.io/syncagent.kcp.io_publishedresources.yaml - regenerated CRD
  • test/crds/subnet.yaml - new test CRD with status subresource
  • test/e2e/sync/related_status_test.go - three e2e tests

Testing

Unit tests (make test) — all pass, including 6 new sub-tests in TestSyncObjectStatusForward and TestSyncObjectContentsForwardStatusRunsEvenOnSpecRequeue

Focused e2e — three new tests covering the happy paths and the default-off behavior:

go test -tags e2e -run "TestSyncRelatedObjectStatus" ./test/e2e/sync/
--- PASS: TestSyncRelatedObjectStatusFromKcp     (29s)
--- PASS: TestSyncRelatedObjectStatusFromService (18s)
--- PASS: TestSyncRelatedObjectStatusNotSyncedByDefault (27s)

Full e2e (make test-e2e) — 38/39 pass; the one failure (TestAPIExportEndpointSliceDifferentCluster) is pre-existing and unrelated to this change.

What Type of PR Is This?

/kind feature

Related Issue(s)

Fixes #173

Release Notes

Status sync for related resources: set syncStatus: true on a RelatedResourceSpec to sync the status subresource in the same direction as the spec. For origin: kcp status flows kcp -> service cluster; for origin: service it flows service cluster -> kcp.

@kcp-ci-bot kcp-ci-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. dco-signoff: yes Indicates the PR's author has signed the DCO. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 29, 2026
@iakmc iakmc force-pushed the sync-status-for-related-resources branch from cfd1dd0 to 64562ba Compare June 29, 2026 23:00
@kcp-ci-bot kcp-ci-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 29, 2026
@iakmc

iakmc commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

/test pull-api-syncagent-verify

@iakmc iakmc force-pushed the sync-status-for-related-resources branch from 64562ba to 31a3e65 Compare June 29, 2026 23:10
@iakmc iakmc force-pushed the sync-status-for-related-resources branch from 31a3e65 to 2d838c5 Compare June 30, 2026 09:06
embik
embik requested changes Jun 30, 2026
View reviewed changes
Comment thread internal/sync/object_syncer.go Outdated
embik
embik reviewed Jun 30, 2026
View reviewed changes
Comment thread internal/sync/object_syncer.go Outdated
embik
embik reviewed Jun 30, 2026
View reviewed changes
Comment thread sdk/apis/syncagent/v1alpha1/published_resource.go Outdated
iakmc added 2 commits June 30, 2026 17:15
@iakmc
Add SyncStatus field to RelatedResourceSpec
2e90f2a 
Signed-off-by: Iskren <iskren@kubermatic.com>

some codeverify fixes

sdk changes

Signed-off-by: Iskren <iskren@kubermatic.com>
@iakmc
handle missing status subresource and add unit tests for syncObjectSt…
79025d5 
…atusForward

Signed-off-by: Iskren <iskren@kubermatic.com>
@iakmc iakmc force-pushed the sync-status-for-related-resources branch from 2d838c5 to 0c638ce Compare June 30, 2026 14:15
@kcp-ci-bot

kcp-ci-bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from embik. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@iakmc

iakmc commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

@embik Thanks for the review. Can you take a look when the e2e tests are finished?

@embik

embik commented Jun 30, 2026

Copy link
Copy Markdown
Member

@embik Thanks for the review. Can you take a look when the e2e tests are finished?

Sure thing! The PR looked ready for review, apologies.

SimonTheLeg
SimonTheLeg reviewed Jun 30, 2026
View reviewed changes

@SimonTheLeg SimonTheLeg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks fine to me. I have one small nit, but that is about it. It took me a while to wrap my head around why we need to jump through all these different cases between spec and status, but yes given that we work with unstructured data mostly and our current architecture, this looks correct to me.

I'll leave the final verdict to @xrstf

Comment thread test/crds/subnet.yaml Outdated
Comment on lines +4 to +8
name: subnets.aws.example.com
spec:
group: aws.example.com
scope: Namespaced
names:

@SimonTheLeg SimonTheLeg Jun 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's please not use any vendor specific language, like "aws" here.
And if it needs to be changed, I would strongly be in favor to renaming this to something like crontabswithstatus. That also clearly indicates to others why we need another CRD for this use case

@iakmc iakmc Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The short version is - spec and status are separate API endpoints even if they look like one object, so they need separate write calls. And that's why status sync always runs even when spec just changed - otherwise you would miss the status update on that cycle and need to wait for next reconciliation.

@iakmc
enforce watch when syncStatus and origin: service; simplify syncObjec…
4a45463 
…tStatusForward

Signed-off-by: Iskren <iskren@kubermatic.com>

resolved nitpick
@iakmc iakmc force-pushed the sync-status-for-related-resources branch from 0c638ce to 4a45463 Compare June 30, 2026 20:16
@iakmc

iakmc commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

/test pull-api-syncagent-test-e2e-kcp-0.30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@embik embik embik requested changes
+1 more reviewer
@SimonTheLeg SimonTheLeg SimonTheLeg left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@embik embik

Labels

dco-signoff: yes Indicates the PR's author has signed the DCO. kind/feature Categorizes issue or PR as related to a new feature. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

feature: sync status of related resources

4 participants

@iakmc @kcp-ci-bot @embik @SimonTheLeg